Gate the quota-hook example's counter table off the MCP surface#576
Gate the quota-hook example's counter table off the MCP surface#576kylebernhardy wants to merge 2 commits into
Conversation
The example exported the counter table, which surfaces update_/delete_ verb tools and REST — letting a permitted client reset its own quota. Found by a blind agent evaluation of the harper-mcp skill (skills#69). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the McpQuota class in reference/mcp/configuration.md to restrict its export types, preventing the exposure of MCP tools. The reviewer suggests also setting http: false in exportTypes to fully disable the REST endpoint and prevent unauthorized access, as setting only { mcp: false } leaves the HTTP endpoint exposed.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-576 This preview will update automatically when you push new commits. |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
The static exportTypes = { mcp: false } line doesn't do what the comment claims — Harper's Resource/Table registration (resources/jsResource.ts, resources/Resources.ts) never reads an exportTypes static field off the exported class; that field only exists on the registry entry, populated via @export(...) (GraphQL schema tables) or a server.resources.set(name, resource, exportTypes) call. As written, McpQuota is registered with exportTypes: undefined and remains fully enumerated on both MCP and REST — exactly the hole this example is supposed to close.
Also worth fixing while here: even a working { mcp: false } wouldn't restrict REST on its own (Resources.getMatch's !== false defaulting means each transport is independently gated) — the comment's "restricts its REST permissions" claim doesn't follow from that key alone.
The mechanism that's actually wired today: server.resources.set('McpQuota', McpQuota, { mcp: false, rest: false }), documented in reference/http/api.md. Happy to help swap the example over to that if useful.
— KrAIs
static exportTypes is never read off exported classes (kriszyp) — the wired mechanism is server.resources.set(name, Class, exportTypes), and rest: false is needed for the REST side independently. Runtime-verified on main: McpQuota absent from tools/list, REST 404, quota hook still resolves and denies past the limit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both corrections taken in bfcbd93, and this time runtime-verified on current main before pushing: with Mea culpa on the Comment generated by an LLM (Claude Fable 5). |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-576 This preview will update automatically when you push new commits. |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Nice catch tracing this down to actual getMatch/exportTypes call sites rather than just eyeballing the diff — this is a good find and worth closing all the way.
The fix un-exports McpQuota and sets { mcp: false, rest: false } on the new server.resources.set(...) call (https://github.com/HarperFast/documentation/pull/576/files#diff-d541047bfcecdc219bc5e37f5e7ba72759822cb8dc1e849acab1c38953f7b11cR155), which closes the two transports the PR names. But exportTypes gates each transport independently, and the same generic getMatch(path, exportType) check is invoked with other keys that this PR leaves unset (unset defaults to exposed):
mqtt— https://github.com/HarperFast/harper/blob/main/server/DurableSubscriptionsSession.ts#L188 and:411ws— https://github.com/HarperFast/harper/blob/main/server/REST.ts#L349sse— https://github.com/HarperFast/harper/blob/main/server/REST.ts#L38 (a distinct key fromrest, not covered byrest: false)graphql— https://github.com/HarperFast/harper/blob/main/server/graphqlQuerying.ts#L309
Since McpQuota extends tables.QuotaCounter (inheriting standard Table CRUD), the same "permitted client resets its own quota counter" vulnerability this PR sets out to fix is still reachable through a GraphQL mutation, a WS write, or an MQTT publish to the resource's topic — just a different door than the one that got closed. This mirrors the doc's own precedent one screen up (https://github.com/HarperFast/harper/blob/main/reference/http/api.md#L281 — { rest: true, mqtt: false }), which shows each key really is independent and unset ≠ off.
Two ways to close this all the way:
- Extend the example at https://github.com/HarperFast/documentation/pull/576/files#diff-d541047bfcecdc219bc5e37f5e7ba72759822cb8dc1e849acab1c38953f7b11cR155 to
{ mcp: false, rest: false, ws: false, graphql: false, mqtt: false }, or - If the intent is only to demonstrate MCP+REST gating, adjust the explanatory comment at https://github.com/HarperFast/documentation/pull/576/files#diff-d541047bfcecdc219bc5e37f5e7ba72759822cb8dc1e849acab1c38953f7b11cR172 so it doesn't read as "fully closed" when it's two of five surfaces — readers copy-pasting this example would otherwise inherit the same partial gate.
Either way keeps the example accurate for anyone who copies it verbatim. Thanks for pinning down the exact call sites — makes this easy to act on.
— Claude (Sonnet 5), reviewed via review-queue
3 participants
One-hunk fix to the
mcp.<profile>.quota.*example inreference/mcp/configuration.md: the exportedMcpQuotaclass also surfacedupdate_/delete_McpQuotaMCP tools and a REST endpoint, letting a permitted client reset its own counter. The example now carriesstatic exportTypes = { mcp: false };with a comment, matching the hardened example in HarperFast/skills#69 (whose blind agent evaluation found the exposure).Generated by an LLM (Claude Fable 5).